feat: add find line segment Intersection algorithm #1705
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1705 +/- ##
==========================================
+ Coverage 84.65% 84.69% +0.03%
==========================================
Files 378 379 +1
Lines 19744 19943 +199
Branches 2951 2981 +30
==========================================
+ Hits 16715 16890 +175
- Misses 3029 3053 +24 ☔ View full report in Codecov by Sentry. |
Geometry/PlaneSweep.js
Outdated
| this.#activeSet.add(segment) | ||
|
|
||
| // Check for intersections with neighboring active segments | ||
| const neighbors = Array.from(this.#activeSet).filter( |
There was a problem hiding this comment.
How is this efficient in general? The active set might very well contain linearly many line segments meaning you get quadratic runtime? The crucial point of this algorithm is that active line segments are managed in a sorted set so you only have to check adjacent line segments in the sorted order.
There was a problem hiding this comment.
I've implemented Bently-Ottmann algorithm and improved the complexity to O((n+k) logn).
There was a problem hiding this comment.
Again, this should not be a class, but rather a single function which takes a list of line segments and produces a list of their intersections (or similar). This should also not be called "plane sweep" since that's just a general technique, but rather "LineSegmentIntersections" or similar.
I'd also recommend testing against the brute-force method to have some more complicated test cases.
saahil-mahato
changed the title
saahil-mahato
changed the title
|
I have made changes. Please review and let me know if anything is missing. Thanks. |
| @@ -0,0 +1,199 @@ | |||
| /** | |||
| * @typedef {Object} Point | |||
There was a problem hiding this comment.
This, and many of the other typedefs (Segment, Event, maybe Intersection) should probably be classes; the functions on them make sense as methods (for example a findIntersection method for Segment).
findIntersections should stay a function however.
If you decide to keep (some of) these as "implicit" "structs" for whatever reason, the create... functions are still useless boilerplate which should be removed (and definitely doesn't need to be tested).
| if (event.types.includes('left')) { | ||
| event.segments.forEach((segment) => { | ||
| sweepLineStatus.push(segment) | ||
| sweepLineStatus.sort(compareSegmentsY) |
There was a problem hiding this comment.
This is O(n log n), resulting in a total time complexity of O(n² log n), which is worse than the naive brute force algorithm.
As said, the sweep line status structure needs to use a sorted set / map data structure. There should be efficient implementations of some in this repo; you need to use one.
| } | ||
|
|
||
| if (event.types.includes('intersection')) { | ||
| // Re-check all pairs of segments at this x-coordinate for intersections |
There was a problem hiding this comment.
This also suffers from poor time complexity: First we do a linear filtering, then we do a quadratic brute force finding of intersections.
| }) | ||
| }) | ||
|
|
||
| describe('calculateIntersectionPoint', () => { |
There was a problem hiding this comment.
Only calculateIntersectionPoint, handlePotentialIntersection and findIntersections really need tests. The rest is practically trivial and also covered transitively by sufficiently complex tests of the latter.
| }) | ||
| }) | ||
|
|
||
| describe('findIntersections', () => { |
There was a problem hiding this comment.
This really needs better, interesting tests with a couple segments and a couple events happening. Ideally even large randomized tests against the brute force algorithm.
|
😕 |
Describe your change:
Checklist:
Example:
UserProfile.jsis allowed butuserprofile.js,Userprofile.js,user-Profile.js,userProfile.jsare notFixes: #{$ISSUE_NO}.